Skip to content

Conversation

@S-Saranya1
Copy link
Contributor

@S-Saranya1 S-Saranya1 commented Oct 28, 2025

This PR adds guidelines for implementing business metrics in the AWS SDK for Java v2, based on team decisions.

@S-Saranya1 S-Saranya1 requested a review from a team as a code owner October 28, 2025 19:36
@S-Saranya1 S-Saranya1 added the changelog-not-required Indicate changelog entry is not required for a specific PR label Oct 28, 2025

### Avoid Request Mutation for Business Metrics

**SHOULD NOT** use request mutation (`.toBuilder().build()`) for adding business metrics as it creates unnecessary object copies and performance overhead.
Copy link
Contributor

@joviegas joviegas Oct 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about high level features which adds the User agent where the features are resolved before the context is built based on the Client settings? Should we also directly add it to the Execution Attribute ? (As in Transfer manager, Batch manager, CrossRegion etc)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yeah, good point to cover! Yes, if there are cases where high-level features are resolved before ExecutionContext is built and we can't access ExecutionAttributes directly.
We have two options: if we can somehow determine the feature from client configuration or execution parameters (like retry mode and protocol detection already do), then we can add it in AwsExecutionContextBuilder in core which avoids request mutation. But, if the feature requires analyzing specific request details that aren't available in client config, then request mutation is acceptable as a fallback ig. Will update the doc about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated it.

@joviegas
Copy link
Contributor

Thanks for creating the guidelines this is very helpful.
Can you mention if there are any guidelines on changing an existing business metric , as in should it be considered backward incompatible or just bumping minor version is fine so that identifying the metrics with new version is easier.

@S-Saranya1
Copy link
Contributor Author

S-Saranya1 commented Oct 29, 2025

Thanks for creating the guidelines this is very helpful. Can you mention if there are any guidelines on changing an existing business metric , as in should it be considered backward incompatible or just bumping minor version is fine so that identifying the metrics with new version is easier.

Changes to existing business metrics should be treated as backward compatible and only require a minor version bump I feel, what do you think? Since business metrics are purely observational, and don't affect SDK functionality or customer code behavior, customer applications remain unaffected, so changes like modifying an existing business metric are safe changes I feel. A minor version bump is sufficient and helps teams track when metric changes were introduced.

@joviegas
Copy link
Contributor

only require a minor version bump I feel, what do

yeap minor version bump looks good

@sonarqubecloud
Copy link

@S-Saranya1 S-Saranya1 added this pull request to the merge queue Oct 31, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 31, 2025
@S-Saranya1 S-Saranya1 added this pull request to the merge queue Oct 31, 2025
Merged via the queue into master with commit ef242e0 Oct 31, 2025
49 of 51 checks passed
@github-actions
Copy link

This pull request has been closed and the conversation has been locked. Comments on closed PRs are hard for our team to see. If you need more assistance, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 31, 2025
@S-Saranya1 S-Saranya1 deleted the somepal/add-business-metrics-code-guidelines branch October 31, 2025 05:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

changelog-not-required Indicate changelog entry is not required for a specific PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants